Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #655 +/- ##
==========================================
- Coverage 78.07% 77.98% -0.10%
==========================================
Files 136 136
Lines 13228 13240 +12
Branches 1991 1993 +2
==========================================
- Hits 10328 10325 -3
- Misses 2063 2075 +12
- Partials 837 840 +3
🚀 New features to boost your workflow:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new ExtractorRunResult.RestartRequired state to the extractor runtime, allowing for clean shutdowns that still necessitate a restart, particularly when a configuration revision changes. The Run and BuildAndRunExtractor methods have been updated to handle this new state, including logging relevant information and setting appropriate backoff. Unit tests were also updated to reduce backoff times and explicitly set the restart policy to OnError for testing config change scenarios. There are no review comments to address.
Hmnt39
left a comment
Hmnt39
left a comment
There was a problem hiding this comment.
Is this restart trigger will only be done on remote config changes or local remote will also do it ?
If connection config is set to local, then it will do it. If connection config is set to 'no connection', Then it will not do it. I think this is correct behaviour. |
Toshad
added
the
waiting-for-risk-review
aasmundeldhuset
left a comment
aasmundeldhuset
left a comment
There was a problem hiding this comment.
🦄
I only have style feedback; the two that are in scope of this PR are small enough that you can ignore them if you want - but I strongly recommend breaking down the large function to make it easier to see the high-level flow (in a separate PR).
| // Otherwise, immediately restart. | ||
| backoff = 0; | ||
| } | ||
| else if (result == ExtractorRunResult.RestartRequired) |
There was a problem hiding this comment.
Not something that needs to be addressed in this PR, but: shouldn't this large if/else over an enum be a switch statement?
| /// The extractor was stopped with a clean shutdown. | ||
| /// But we need to restart it (possibly due to a revision change). | ||
| /// </summary> | ||
| RestartRequired |
There was a problem hiding this comment.
Nitpick that you don't have to address, but: adding a comma at the end will prevent an unnecessary modification of this line the next time an enum member is added.
| { | ||
| using var internalTokenSource = CancellationTokenSource.CreateLinkedTokenSource(_source.Token); | ||
| TExtractor extractor; | ||
| var shouldRestart = false; |
There was a problem hiding this comment.
This isn't used in the first try, so I think it should be declared above the second one, so that its limited scope is more clear.
| return ExtractorRunResult.Error; | ||
| } | ||
|
|
||
| if (shouldRestart) |
There was a problem hiding this comment.
Add a blank line above to visually differentiate this from the try/catch.
On a related note: this is a 100-line function that has got three quite distinct parts: the creation of the extractor, the waiting for the extractor or the "revision" change event, and the final restart check. I recommend splitting out the first two into separate functions - but that doesn't need to be done in this PR.
aasmundeldhuset
added
waiting-for-team
3 participants
Currently, we restart on config change only if restart policy is 'always' but we also want to restart for other cases to support integrations.